-
Notifications
You must be signed in to change notification settings - Fork 199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conditional type support #105
Conversation
Let me know when this is ready for review. |
This is needed because for isAssignableTo checks these types works differently
I think it's ready now. I did the following changes:
|
Supporting enums in conditional type handling now so this PR also fixes #71 |
Can you merge master/rebase? |
I'm still completely stuck here and I'm not so sure anymore that the problem with the lost annotations are actually caused by the conditional types. I can't reproduce these problems when using conditional types without a mapping type. And I don't really understand the annotation parsing in the generator and why all this is working with some mapped types at all. So what are our options? Would be a shame not to merge this just because some complex scenarios are not fully working. When looking at the generated vega-lite schema it actually looks pretty good when you ignore the fact that some annotations are lost and some enum types are dereferened (Couldn't re-produce this outside of vega-lite yet). |
/**
* Hello world
*/
type LabelOpacity = string | number
export type MyObject = LabelOpacity extends number ? never : LabelOpacity; The comment gets lost here as well {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"type": "string"
}
}
} Maybe there is a better way to write the TypeScript. I agree that it would be a shame to not merge this. Let's try for a bit more but I am definitely not against merging this even if Vega-Lite doesn't work yet. |
With /**
* Hello world
*/
type LabelOpacity = string | number;
export type MyObject = Exclude<LabelOpacity, number>; we get the comment from {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"description": "Exclude from T those types that are assignable to U",
"type": "string"
}
}
} Which is not all that helpful. |
I suspect what is happening is that we are looking up the docs from the conditional type but we usually care more about the comments on the thing we excluded from. Maybe we can have a special case for Exclude where we ignore its comment and go straight to the type we exclude from? Both of these /**
* Hello world
*/
type OurString = string;
type LabelOpacity = OurString | number;
export type MyObject = Exclude<LabelOpacity, number>; /**
* Hello world
*/
type LabelOpacity = string | number;
export type MyObject = Exclude<LabelOpacity, number>; should produce {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"description": "Hello world",
"type": "string"
}
}
} /**
* Hello world
*/
export type OurString = string;
type LabelOpacity = OurString | number;
export type MyObject = Exclude<LabelOpacity, number>; would still produce, this, though
|
This case actually works /**
* Hello world
*/
type OurString = string;
type LabelOpacity = OurString | number;
export type MyObject = LabelOpacity extends number ? never : LabelOpacity; {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"description": "Hello world",
"type": "string"
}
}
} So one way would be to move the comments to the remaining type. However, it would be nice to use the comment from the type that we remove something from (here |
@kayahr do these small use cases help you? I would love to merge this feature soon and make a new release. |
Yes, I think they are helpful. I ran out of time for now but I definitely want to look into it. |
Great. Btw, my offer to send you chocolate to thank you for adding caching still stands. |
So you were serious about this? :) Well, thanks for the offer but it is really not necessary. And chocolate isn't healthy anyway :) |
Your first example is fixed now: /**
* Hello world
*/
type LabelOpacity = string | number
export type MyObject = LabelOpacity extends number ? never : LabelOpacity; This wasn't actually a jsdoc issue. The result type was just wrong. TypeScript only narrows down the result type if it is a type parameter matching the check type. This isn't the case here so the result type must be LabelOpacity and not string. With this fixed the JSDoc from LabelOpacity is now automatically inherited by MyObject. I'll continue now with your next example (In case you wonder, I'm giving them thumbs-up when finished to keep track) |
Yes, it's useless to copy the description of the Exclude type and we should ignore this somehow but I disagree on going straight to the type we exclude from. We should use the actual result type instead. Let's take a look at your examples again:
The result of the Exclude type here is
The result of this Exclude is |
Any good idea how to safely detect the internal TypeScript |
For now I've added a "not-so-nice" hack. Because I guess ignoring Exclude is not enough because there are many other types in the typescript libs from which we don't want to use the comments I'm ignoring now all comments coming from nodes of the typescript libs by checking the source file path with a regular expression. If you know a better solution then please tell me. With this last fix all the examples you mentioned are working with the already explained exception of not using the comment from a union type which was narrowed down by an exclude because in my opinion that would be wrong. So this: /** Description A */
type A = string;
/** Description B */
type B = number;
/** Description AorB */
type AorB = A | B;
/** Description StringOrNumber */
type StringOrNumber = string | number;
export type MyObject = {
a?: Exclude<AorB, B>;
b?: Exclude<AorB, A>;
c?: Exclude<AorB, boolean>;
d?: Exclude<StringOrNumber, string>;
}; Now becomes this: {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"additionalProperties": false,
"properties": {
"a": {
"description": "Description A",
"type": "string"
},
"b": {
"description": "Description B",
"type": "number"
},
"c": {
"anyOf": [
{
"description": "Description A",
"type": "string"
},
{
"description": "Description B",
"type": "number"
}
],
"description": "Description AorB"
},
"d": {
"type": "number"
}
},
"type": "object"
}
}
} Notice that property "d" has no description because type "number" has no comment. And in my opinion this is correct. |
I tried to compile the Vega-Lite schema but am now running into an issue with the stack size. If I set the stack size to 10000 (smaller values are not enough) then I get a segfault. Apparently, that's because node's You can reproduce the issue by going into |
Conditional doesn't actually seem to work in mapped types anymore. For example, if you use export interface Axis {
/**
* The label opacity.
*
* @minimum 0
*/
labelOpacity: number | string;
}
type OmitString<T> = {
[P in keyof T]: Exclude<T[P], string>
};
export type MyObject = OmitString<Axis>; it produces (note how the {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"additionalProperties": false,
"properties": {
"labelOpacity": {
"description": "The label opacity.",
"minimum": 0,
"type": [
"number",
"string"
]
}
},
"required": [
"labelOpacity"
],
"type": "object"
}
}
} |
Hm? I just tried your example and it works fine here: {
"$ref": "#/definitions/MyObject",
"$schema": "http://json-schema.org/draft-07/schema#",
"definitions": {
"MyObject": {
"additionalProperties": false,
"properties": {
"labelOpacity": {
"type": "number"
}
},
"required": [
"labelOpacity"
],
"type": "object"
}
}
} And BTW: The conditional type isn't the reason here why the comment of the property gets lost. That's a problem of the mapping type. With a mapping like the one below you also lose all comments without conditional types to be involved so that should be considered a separate bug: type StringsEverywhere<T> = {
[P in keyof T]: string
}; |
I think the stack overflow is an unrelated problem. Yes, the schema generator is much more under stress when the millions of conditional types in vega-lite are processed now but in the end I think the problem is here. This code recursively generates a very large array. I checked the content and noticed that there are a lot of duplicates in it. And I'm not speaking about equal objects with same ids, I speak about exactly the same objects. I don't think this makes sense and it can be fixed like this: children.push(...new Set(this.childTypeFormatter.getChildren(type))); Maybe an even better solution would be to change the return type of this After fixing this the stack overflow is gone and you get a new error:
There are indeed two different types in Vega named So both problems seems to be unrelated to this PR: |
Sorry, wrong program. Try this one. export interface Axis {
/**
* The label opacity.
*
* @minimum 0
*/
labelOpacity: number | string;
}
type OmitString<T> = {
[P in keyof T]: T[P] extends string ? never : T[P]
};
export type MyObject = OmitString<Axis>; But now I am realizing that the issue is Typescript itself. Alright, lets merge this and keep iterating. |
Ref #100
See notes in #100 (comment)